-
Notifications
You must be signed in to change notification settings - Fork 10.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make sure expression before &&
is always boolean in React rendering in marketing page
#37227
Conversation
@eason9487 , this fixes issue #37175 that you brought up during code review on my PR in #37044 (review). Could you help to review this please? 🙏 |
Test Results SummaryCommit SHA: dd6856a
To view the full API test report, click here. To view the full E2E test report, click here. To view all test reports, visit the WooCommerce Test Reports Dashboard. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## trunk #37227 +/- ##
==========================================
- Coverage 46.7% 46.7% -0.0%
Complexity 17191 17191
==========================================
Files 429 429
Lines 64845 64865 +20
==========================================
+ Hits 30275 30284 +9
- Misses 34570 34581 +11
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing the render 0
issue.
The main issue mentioned in #37044 (review) has been fixed.
However, adding additional type casting might not be such a necessary precaution for some very deterministic and single types such as boolean or string.
In some cases where we are using a variable like
shouldShowExtensions
andisModalOpen
which should already be boolean, we use!!
in front of them anyway, so that we don't need to second guess it when we look at it, and we are future proofing it (e.g. when the variable type is changed from boolean to another type in the future).
The only falsy values that will be rendered are 0
and NaN
, i.e., only the number type. When reading these !!
, it will instead think one more time: Why it needs an additional type casting when no number type result will happen here?
Furthermore, assuming this was a good practice: "Always add a type casting to any type, for example, string, to prevent it from rendering a 0
in the future if it is changed to another type.", then another question it brings up would be: "Do all rendering statements like the following have to start with a precautionary check in front of them?"
woocommerce/plugins/woocommerce-admin/client/marketing/components/knowledge-base/index.js
Line 88 in dd6856a
<h3>{ post.title }</h3> |
woocommerce/plugins/woocommerce-admin/client/marketing/components/knowledge-base/index.js
Line 91 in dd6856a
{ post.author_name } |
Line 87 in dd6856a
{ el.name } |
Line 90 in dd6856a
{ el.description } |
Line 32 in dd6856a
{ registeredChannel.issueText } |
There are very many such rendering statements. If these rendering statements are still considered not to require a precaution, then conversely, are those similar changes made in this PR still needed when there is clearly no problem now?
@eason9487 , thanks for the review. 🙏
The short answer is that based on https://reactjs.org/docs/jsx-in-depth.html?#booleans-null-and-undefined-are-ignored:
So I use Here's another thinking that I have. When I look at the issue, I did a search for By looking at the search result, with the In the future, I'm hoping to get
But the examples you gave does not involve the use of |
Full description in the doc is:
I think the full description is telling how to fix the issue but doesn't mean to be a practice for all conditional rendering statements.
I meant if it's important to prevent rendering For example, consider the following code: return (
<div>
Statement A:
{ el.description && (
<FlexItem>
{ el.description }
</FlexItem>
) }
Statement B:
{ !! el.description && (
<FlexItem>
{ el.description }
</FlexItem>
) }
Statement C:
{ el.description }
</div>
); Although TS has ensured that the They have the same situation with the practice "to avoid rendering
When reading code, the need to guess depends on the reader's expected familiarity with React, rather than whether a line of a statement looks like it 100% won't have this issue at all. I could also say that when seeing But, when seeing
This practice assures it won't have this PR's issue but it brings new questions to my mind, and then I still need to dig around. Overall it may not make for better readability or inferability. In addition, by searching the codebase (5ed070d) with the following regex patterns, I don't really think this codebase needs this practice.
Perhaps what can be summarized are:
IMO, this linting rule is mostly needed for repos powered by React Native, but it's verbose for this repo. |
While I don't recommend adding redundant type castings for conditional rendering statements that obviously won't render out an unexpected |
@eason9487 , thanks so much for sharing your thoughts! 🙂 🙏
To me, Statement C is expected to render whatever is given in
Just want to say that I especially appreciate this perspective, I didn't think of it this way, thanks for sharing this. 🙂 And thanks for approving this PR. I will proceed to merge it. Maybe as I work on this more, I will get to understand your point better and might come back to revert some of the code change here that I made. 😂 |
@eason9487 , I can open a PR to change the |
I must clarify that if a variable that was always a string becomes possible to be a number, then this does not mean that the current implementation will be the expected result. Whether or not rendering But either one is a false statement. How it should be rendered depends on the demand when it occurs, not on the current one-sided assumptions. Moreover, with the premise that Even if C has no explicit condition, it still has an implicit rendering condition and is identical to A B, i.e., the empty string is not rendered (no content to render). This can not be used to infer that "C should render whatever the value is given.", especially the Otherwise, if the assertion was true that "it's expected to render whatever is given", then you probably also meant that rendering out a |
It would be nice and appreciated.
I would still suggest removing unnecessary If the type of a variable can be narrowed down to a very specific type, it is better practice to ensure this at the beginning, because all the statements associated with it will become easier to code and better to read. In the case of // Change this
const shouldShowExtensions =
getAdminSetting( 'allowMarketplaceSuggestions', false ) &&
currentUserCan( 'install_plugins' );
// to
const shouldShowExtensions =
Boolean( getAdminSetting( 'allowMarketplaceSuggestions', false ) ) &&
currentUserCan( 'install_plugins' ); |
Ahh of course! I should have thought of that! 😂 Okay, I can work with that. By the way, you were using // Using Boolean:
const shouldShowExtensions =
Boolean( getAdminSetting( 'allowMarketplaceSuggestions', false ) ) &&
currentUserCan( 'install_plugins' );
// Using !!:
const shouldShowExtensions =
!! ( getAdminSetting( 'allowMarketplaceSuggestions', false ) ) &&
currentUserCan( 'install_plugins' ); |
This is just my coding preference, the main reason is that compared to |
@eason9487 , I have created PR #37452 for the above. Let me know what you think. 🙏 |
All Submissions:
Changes proposed in this Pull Request:
Closes #37175.
In this PR, we make sure that the expression before
&&
is always boolean by using double negation!!
.In some cases where we are using a variable like
shouldShowExtensions
andisModalOpen
which should already be boolean, we use!!
in front of them anyway, so that we don't need to second guess it when we look at it, and we are future proofing it (e.g. when the variable type is changed from boolean to another type in the future).How to test the changes in this Pull Request:
Functionality test:
&&
is always boolean in JSX #37175.Code check:
&&
in React rendering is always boolean.!
,!!
, or a comparison operator like===
or>=
.Other information:
pnpm --filter=<project> changelog add
?FOR PR REVIEWER ONLY: